Repository navigation
test: backfill coverage and restore the 100% gate - #81
JacobCoffee wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I checked whether the bare DecimalField() in get_session_rating_analytics (reports_analytics.py:215) could still fail or return the wrong type depending on the database backend. I ruled it out: the result goes through Decimal(str(...)) right away, so it is normalized whatever type the backend returns.
Extended reasoning...
The ruled-out concern was that a DecimalField with no precision might be converted differently by each database backend. The next line wraps the aggregate in Decimal(str(...)), so the value comes out as a Decimal either way.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa41eade98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fa41ead to
46a877f
Compare
There was a problem hiding this comment.
The latest push fixes the issue I raised earlier: _forms_valid now saves the conference and its KPI targets inside one transaction.atomic() block, and sends the success message only after both saves commit. I found no new bugs, but a human should still review this before merge. It adds about 5,600 lines of tests, and the earlier third-party comment on the fail_under line is not visible to me, so I can't confirm it was resolved.
What was reviewed:
ConferenceEditView:post()goes only through_forms_valid,transactionis already imported, and the removedform_validoverride no longer runssuper().form_valida second time.get_session_rating_analytics:Avg(..., output_field=DecimalField())now matches the DecimalValue(_ZERO)fallback, and the result is still normalized withDecimal(str(...)).- The badge font test now uses reportlab's bundled
Vera.ttfinstead of the macOS-only Geneva path.
Extended reasoning...
This change raises the coverage gate in pyproject.toml from 94 to 100 and adds about 5,600 lines of new tests. It also makes two small production fixes in src/django_program/manage/views.py (one atomic save for the conference and KPI forms) and src/django_program/manage/reports_analytics.py (an output_field on Avg). It touches no auth, crypto, or injection-sensitive code. I deferred because the change is large, and because a third-party inline comment on the fail_under line has an author reply whose content I can't see, so I can't confirm that thread was addressed.
c096e4e to
dc87137
Compare
There was a problem hiding this comment.
I reviewed the latest push and didn't find any bugs. The earlier KPI-save concern is fixed: both saves now run inside one transaction.atomic(), and the success message is sent only after that block commits. The PR still adds about 5,600 lines of tests and raises the coverage gate to 100%, which I couldn't confirm by running the suite, so a human look would still be worthwhile.
What was reviewed:
ConferenceEditView._forms_validinviews.py:transactionwas already imported, and nothing else depended on the removedform_validoverride.Avg("score", output_field=DecimalField())inreports_analytics.py: this now matches the type of the Decimal zeroValueinsideCoalesce.- The badge font test in
test_badges.pynow uses reportlab's bundled Vera.ttf, so it no longer depends on a macOS-only font path.
Extended reasoning...
Apart from about 5,600 lines of new test modules, this push changes two production files with a few lines each: a Coalesce type fix in the reports analytics and an atomic conference/KPI save in ConferenceEditView. It touches no auth, permissions or injection-sensitive code. The latest commit addresses my earlier inline finding. I deferred because the change is large and depends on the suite reaching 100% coverage, which I couldn't run here to check. The other bot's comment on the coverage gate in pyproject.toml was resolved only by the PR author.
228b167 to
0a51263
Compare
There was a problem hiding this comment.
The latest push fixes the partial-save case I raised earlier on ConferenceEditView: the conference and KPI saves now run in one transaction.atomic() block, and the success message is added only after it commits. I didn't find any new bugs. A human should still look, because the earlier automated thread on the coverage gate at pyproject.toml:97 was resolved by the author, and about 5,600 lines of new tests is a lot to approve automatically.
What was reviewed:
_forms_validin manage/views.py:transactionis already imported, and the duplicate conference save from the removedform_validoverride is gone.Avg("score", output_field=DecimalField())in reports_analytics.py: its type now matches the DecimalValueinCoalesce.- The badge font test now uses reportlab's bundled Vera.ttf instead of the macOS-only Geneva font.
Extended reasoning...
The production changes are two small fixes. One makes the conference and KPI targets save atomic in ConferenceEditView. The other sets an explicit DecimalField output type on an Avg aggregate. The rest of the change is about 5,600 lines of new test files, plus raising the coverage gate in pyproject.toml from 94 to 100. None of it touches auth, permissions or crypto. I deferred instead of approving because an earlier automated reviewer thread on the pyproject.toml gate was resolved only by the author, and the test additions are too large for an automatic approval.
There was a problem hiding this comment.
LGTM. The latest push fixes the earlier point about ConferenceEditView: the conference and KPI saves now run in one transaction.atomic() block, and the success message is sent only after both commit.
What was reviewed:
_forms_validinmanage/views.py:transactionis already imported, the response from a singlesuper().form_validis returned, and the unreachableform_validoverride is gone.get_session_rating_analytics: givingAvgaDecimalFieldoutput fixes the mixed-typeCoalesceerror, and theDecimal(str(...))cast after it still works.test_badges.py: the font test now uses reportlab's bundled Vera.ttf, so it runs on Linux CI. Without that, the newfail_under = 100gate would fail there.
Extended reasoning...
About 5,400 of the ~5,600 added lines are new test modules. The rest is the coverage gate going from 94 to 100 and two small source fixes in manage/views.py and manage/reports_analytics.py. None of it touches auth, permissions, crypto, injection or data exposure. The latest commit addresses the open atomicity comment on views.py. It also makes the badge font test portable, which plausibly answers the author-resolved thread on the pyproject.toml coverage gate. There is no CODEOWNERS file, and the source changes are small and self-contained.
…ference - fix get_session_rating_analytics crash (mixed Float/Decimal in Coalesce) - drop unreachable ConferenceEditView.form_valid; success message now sent from the path that runs
…t test - a failed KPI save no longer leaves the conference edit committed - font registration test uses reportlab's bundled Vera.ttf instead of a macOS-only system font, so the 100% gate holds on Linux
0a51263 to
7808181
Compare
There was a problem hiding this comment.
I reviewed the latest push and found no bugs. The earlier note on ConferenceEditView is addressed: both saves now run in one transaction.atomic() block, and the success message is sent only after that block finishes. I'm not approving because an automated reviewer's thread on pyproject.toml (the fail_under = 100 line) was resolved by the author, and I can't tell from here whether its point was handled.
What was reviewed:
_forms_validinmanage/views.py: the duplicateform_validoverride is gone, a KPI save failure now rolls back the conference update, andtransactionwas already imported.get_session_rating_analytics:Avg(..., output_field=DecimalField())makes bothCoalescearguments Decimal, which fixes the mixed-typeFieldError.- The badge font test now uses reportlab's bundled
Vera.ttfinstead of a macOS-only font, so it runs on every platform.
Extended reasoning...
The PR is almost entirely new test files (about 5,600 lines), plus two small production fixes in manage/reports_analytics.py and manage/views.py and a coverage gate raised from 94 to 100 in pyproject.toml. It touches no security-sensitive surface such as auth, permissions or injection. The latest commit wraps the conference and KPI saves in one transaction, which resolves the concern I raised earlier. Approval is held back because another bot's comment on pyproject.toml:97 was resolved only by the PR author, so it can't be confirmed as addressed.
Stacked on #80. main sits at 94% line coverage (the coverage job never ran while main's tests were red), so #80 ratchets
fail_underto 94; this PR brings the suite to 100% and restoresfail_under = 100.Tests
New test files only (no changes to existing tests): sponsors services/views/admin/models, manage views / bulk purchases / letters / financial / reports / analytics, terminal and Stripe client, webhooks, purchase orders, check-in, registration admin, conference/pretalx/programs model
__str__s,setup_groups. External boundaries (Stripe SDK, PSF API client) are mocked; everything else runs against the DB.Production changes found while covering
get_session_rating_analyticscrashed on every call (FieldError: Expression contains mixed types: FloatField, DecimalFieldfromCoalesce(Avg("score"), Value(Decimal)));Avgnow declaresoutput_field=DecimalField().ConferenceEditView.form_validwas unreachable (post()goes through_forms_valid, which callssuper().form_valid), so its success message never showed; removed it and send the message from_forms_valid.Verification
uv run python -m pytest -n auto --cov=src/django_program: 2460 passed, 100% (12391/12391).